New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add workflow to check for required reviews #370

Closed

jfrost-mo wants to merge 3 commits into main from triplicate_review_required

Member

jfrost-mo commented Jan 18, 2024 •

edited

Loading

Fixes #366.

Need to test that it passes after enough approving reviews.
Is 2 reviews the right number? 3 would cover science, technical, and portability, but who can do a technical review for my own PRs?

jfrost-mo added the cleanup label

jfrost-mo self-assigned this

Contributor

github-actions bot commented Jan 18, 2024 •

edited

Loading

This comment was marked as outdated.

Sign in to view

jfrost-mo force-pushed the triplicate_review_required branch from f37ed0a to f86a53a Compare

January 18, 2024 14:49

jfrost-mo added full_review bug and removed full_review bug labels

jfrost-mo marked this pull request as ready for review

January 18, 2024 16:21

This comment was marked as outdated.

Sign in to view

jfrost-mo added full_review bug and removed bug full_review labels

jfrost-mo force-pushed the triplicate_review_required branch from 903f9c2 to 1e3251b Compare

January 18, 2024 16:51

jfrost-mo added bug and removed bug labels

jfrost-mo force-pushed the triplicate_review_required branch from 3af646f to 438f600 Compare

January 18, 2024 16:55

This comment was marked as outdated.

Sign in to view

jfrost-mo force-pushed the triplicate_review_required branch from 438f600 to 536d9dd Compare

January 18, 2024 17:07

jfrost-mo added full_review and removed full_review labels

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

jfrost-mo removed the full_review label

This comment was marked as outdated.

Sign in to view

jfrost-mo added the full_review label

This comment was marked as outdated.

Sign in to view

jfrost-mo added full_review and removed full_review labels

jfrost-mo added 3 commits

January 30, 2024 09:32


          Get GitHub token from secrets

7f84b54


          Add workflow to check for required reviews

98dfed6

Add review checking script

Simplify with workflow event

Remove condition for approval

Also run when unlabeled

Colourise output


          Only run on pull_request_review

506193d

The run on pull_request is treated as a seperate check and isn't
updated when a review is actually made.

jfrost-mo force-pushed the triplicate_review_required branch from 57f9e49 to 506193d Compare

January 30, 2024 09:32

Member Author

jfrost-mo commented Jan 30, 2024 •

edited

Loading

The workflow might need to run on pull_request, as currently there are a lot of circumstances where it doesn't run. At least it terminates early, so it shouldn't slow things down over running only on a review being submitted.

The issue with running on pull_request is that it won't rerun after the reviews, so it would still stay failed after the needed reviews had been gathered...

Contributor

Sylviabohnenstengel commented Feb 21, 2024

I think for most changes 2 reviews should be sufficient as 3 reviews but slow process down too much and stop users from adotping the tool if overhead is too much.

Member Author

jfrost-mo commented Feb 23, 2024

I'm tempted to drop this PR as it has a few edge cases it can break on, which fixing would make it significantly more complicated. Instead we should just go for an honour system, and note at the top of the PR if it requires multiple reviews.

jfrost-mo closed this

jfrost-mo deleted the triplicate_review_required branch

March 4, 2024 09:34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup full_review